Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Swap libsignal-protocol dependency with libsignal-client #83

Merged
merged 24 commits into from
Apr 30, 2021

Conversation

rubdos
Copy link
Member

@rubdos rubdos commented Apr 20, 2021

Fixes #72

@rubdos rubdos force-pushed the libsignal-client branch from 73a9ce5 to c4110d2 Compare April 20, 2021 18:32
@rubdos
Copy link
Member Author

rubdos commented Apr 20, 2021

It's a bit of a rewrite, so I'm pushing broken commits as I pass over code. In the end I'll squash the part x things to something with a real commit message :-)

@rubdos
Copy link
Member Author

rubdos commented Apr 26, 2021

Important detail. libsignal-client is AGPLv3. We have to move to AGPLv3 from this point.

@Be-ing
Copy link

Be-ing commented Apr 27, 2021

No, relicensing to AGPL is not needed and can't be done without explicit agreement of everyone who has contributed to this repo:

The ordinary GNU General Public License and the GNU Affero General Public License are two different copyleft licenses, so they are naturally incompatible. We have set up a special kind of explicit compatibility between them: you can include source code under the GNU GPL version 3 together with other source code under the GNU Affero GPL in a single combined program. This is permitted because both of those licenses explicitly say so, and the effect is that the GNU AGPL applies to the combined program. However, you can't simply relicense code from the GNU GPL (with or without “or later”) to the GNU Affero GPL, or vice versa; neither of these licenses gives permission for that. Note also that the GNU Affero GPL version 3 is not a “later version” of the ordinary GNU GPL version 2, because the GNU Affero GPL and the GNU GPL are two different series of licenses.

https://www.gnu.org/licenses/license-compatibility.html

Considering only 10 people have contributed code to this repository so far, I think relicensing to AGPL is feasible. I think it would be advisable to avoid confusion. Also, if we want to copy any of this code to send upstream to libsignal-client, they might want it to be AGPL.

@rubdos
Copy link
Member Author

rubdos commented Apr 27, 2021

So either we relicense the whole thing, or we go the complicated route as I took with Whisperfish: old source is GPLv3 and new source is AGPLv3. If we don't get the permission of them all, that's what we should do.

I'll make an announcement on issue #72.

Either way, the whole of libsignal-service-rs will fall under AGPLv3, since it's a combination of (A)GPLv3 and AGPLv3 sources.

And wrt. sending to libsignal-client: that's a whole other story, because they require a CLA. AFAICT, for moving code, only my code and @gferon's code is being moved on the short term.

@rubdos rubdos marked this pull request as ready for review April 27, 2021 16:25
Copy link
Member Author

@rubdos rubdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're not far off! :-)

@@ -7,8 +7,8 @@ license = "GPLv3"
readme = "../README.md"

[dependencies]
libsignal-protocol = { git = "https://github.com/Michael-F-Bryan/libsignal-protocol-rs" }
zkgroup = { git = "https://github.com/signalapp/zkgroup" }
libsignal-protocol = { git = "https://github.com/whisperfish/libsignal-client", branch = "make-error-sync" }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we wait on signalapp/libsignal#279?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we will wait.

libsignal-service/src/account_manager.rs Show resolved Hide resolved
libsignal-service/src/account_manager.rs Outdated Show resolved Hide resolved
libsignal-service/src/account_manager.rs Outdated Show resolved Hide resolved
libsignal-service/src/cipher.rs Show resolved Hide resolved
libsignal-service/src/lib.rs Outdated Show resolved Hide resolved
libsignal-service/src/push_service.rs Show resolved Hide resolved
Comment on lines 464 to 484
if let Some(ref uuid) = recipient.uuid {
self.cipher.store_context.delete_session(
&libsignal_protocol::Address::new(
uuid.to_string(),
*extra_device_id,
),
)?;
if let Some(_uuid) = recipient.uuid {
unimplemented!("deleting session: unimplemented following the switch to the Rust version of libsignal-protocol");
}
if let Some(e164) = recipient.e164() {
self.cipher.store_context.delete_session(
&libsignal_protocol::Address::new(
&e164,
*extra_device_id,
),
)?;
if let Some(_e164) = recipient.e164() {
unimplemented!("deleting session: unimplemented following the switch to the Rust version of libsignal-protocol");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracking issue for this? This definitely affects Whisperfish.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, I swear I reverted that. I added SessionStoreExt so that's possible again.

libsignal-service/src/sender.rs Outdated Show resolved Hide resolved
libsignal-service/src/account_manager.rs Show resolved Hide resolved
libsignal-service/src/account_manager.rs Outdated Show resolved Hide resolved
libsignal-service/src/cipher.rs Show resolved Hide resolved
libsignal-service/src/lib.rs Outdated Show resolved Hide resolved
libsignal-service/src/account_manager.rs Outdated Show resolved Hide resolved
Comment on lines 464 to 484
if let Some(ref uuid) = recipient.uuid {
self.cipher.store_context.delete_session(
&libsignal_protocol::Address::new(
uuid.to_string(),
*extra_device_id,
),
)?;
if let Some(_uuid) = recipient.uuid {
unimplemented!("deleting session: unimplemented following the switch to the Rust version of libsignal-protocol");
}
if let Some(e164) = recipient.e164() {
self.cipher.store_context.delete_session(
&libsignal_protocol::Address::new(
&e164,
*extra_device_id,
),
)?;
if let Some(_e164) = recipient.e164() {
unimplemented!("deleting session: unimplemented following the switch to the Rust version of libsignal-protocol");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, I swear I reverted that. I added SessionStoreExt so that's possible again.

@Leptopoda
Copy link

So either we relicense the whole thing, or we go the complicated route as I took with Whisperfish: old source is GPLv3 and new source is AGPLv3. If we don't get the permission of them all, that's what we should do.

I'll make an announcement on issue #72.

Either way, the whole of libsignal-service-rs will fall under AGPLv3, since it's a combination of (A)GPLv3 and AGPLv3 sources.

And wrt. sending to libsignal-client: that's a whole other story, because they require a CLA. AFAICT, for moving code, only my code and @gferon's code is being moved on the short term.

Just FYI that's exactly the reason why they have the CLA. They want to own the code so they can release it under a different license or sell it as closed source (like they did to Facebook or so)

@rubdos
Copy link
Member Author

rubdos commented Apr 28, 2021

Just FYI that's exactly the reason why they have the CLA. They want to own the code so they can release it under a different license or sell it as closed source (like they did to Facebook or so)

Yes, and that's exactly the reason that I'm against CLA's too. In my opinion, the person that wrote the code should have the right to choose whether a change of license is okay with them, like in this case :-)

But let's stick to topic, we're very close having libsignal-client replacing libsignal-protocol-rs! 🎉

@gferon
Copy link
Collaborator

gferon commented Apr 28, 2021

Maybe you mean libsignal-protocol by libsignal-protocol? 😂 Both the pure C-bindings crate and the newer one have the same name.

@rubdos
Copy link
Member Author

rubdos commented Apr 28, 2021

potato tomato!

@gferon
Copy link
Collaborator

gferon commented Apr 29, 2021

@rubdos I think this is ready for a second (and hopefully final?) look!

Copy link
Member Author

@rubdos rubdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly clippy, there are still some unresolved discussions, @gferon.

Generally looking good, let's catch everything else in prod ;p

@@ -78,9 +75,12 @@ impl<Service: PushService> AccountManager<Service> {
/// Equivalent to Java's RefreshPreKeysJob
///
/// Returns the next pre-key offset and next signed pre-key offset as a tuple.
pub async fn update_pre_key_bundle(
pub async fn update_pre_key_bundle<R: rand::Rng + rand::CryptoRng>(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub async fn update_pre_key_bundle<R: rand::Rng + rand::CryptoRng>(
#[allow(clippy::too_many_arguments)] // See issue https://github.com/Michael-F-Bryan/libsignal-service-rs/issues/92
pub async fn update_pre_key_bundle<R: rand::Rng + rand::CryptoRng>(

signed_pre_key: signed_pre_key.into(),
identity_key: identity_key_pair.public(),
signed_pre_key: signed_prekey_record.try_into()?,
identity_key: identity_key_pair.public_key().clone(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
identity_key: identity_key_pair.public_key().clone(),
identity_key: identity_key_pair.public_key(),

libsignal-service/src/cipher.rs Outdated Show resolved Hide resolved
libsignal-service/src/push_service.rs Show resolved Hide resolved
libsignal-service/src/push_service.rs Outdated Show resolved Hide resolved
libsignal-service/src/sealed_session_cipher.rs Outdated Show resolved Hide resolved
libsignal-service/src/sender.rs Outdated Show resolved Hide resolved
libsignal-service/src/sender.rs Outdated Show resolved Hide resolved
Comment on lines 706 to 710
if !self
.cipher
.store_context
.contains_session(&recipient_address)?
.session_store
.load_session(&recipient_address, None)
.await?
.is_some()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion feature on GH doesn't let me, but this should be self.[..].is_none() instead, apparently

libsignal-service/src/sender.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider/switch to libsignal-client
5 participants